Revamp oauth scope handling; support requiring multiple scopes.#147
Revamp oauth scope handling; support requiring multiple scopes.#147inklesspen merged 1 commit intocloudendpoints:masterfrom inklesspen:scopes
Conversation
| # pylint: enable=unused-argument | ||
|
|
||
|
|
||
| def _process_scopes(scopes): |
There was a problem hiding this comment.
Maybe document what scopes is?
I believe scopes should be a list of string, and each string is a space-separated list of scope.
Is this correct?
There was a problem hiding this comment.
Oh yeah, whoops!
| _logger.debug('Unable to get authorized scopes.', exc_info=True) | ||
| return | ||
| if not _are_scopes_sufficient(authorized_scopes, sufficient_scopes): | ||
| _logger.debug('Authorized scopes did not satisfy scope requirements.') |
There was a problem hiding this comment.
Shouldn't we return or fail here?
Also, would it be useful to include the actual authorized/sufficient scopes in the log message?
There was a problem hiding this comment.
Yeah, we should. Nice catch.
Honestly, I'm considering removing these lines completely, or attaching them to a separate logger which is normally disabled. These log lines will come up a lot for unauthorized users, so people probably won't want to see them unless they're debugging auth issues.
| _logger.debug('Unable to get authorized scopes.', exc_info=True) | ||
| return | ||
| if not _are_scopes_sufficient(authorized_scopes, sufficient_scopes): | ||
| _logger.debug('Authorized scopes did not satisfy scope requirements.') |
| return None | ||
| os.environ[_ENV_USE_OAUTH_SCOPE] = ' '.join(authorized_scopes) | ||
| _logger.debug('Returning user from matched oauth_user.') | ||
| return |
There was a problem hiding this comment.
I guess this return isn't strictly necessary.
This code should properly handle a situation where multiple scopes are necessary for access.
2 participants
This code should properly handle a situation where multiple scopes are necessary for access.